[MCP Bundle] Fix dependency injection and method-based / invokable support #757
Conversation
camilleislasse
requested review from
Nyholm,
OskarStark and
chr-hertel
as code owners
carsonbot
added
Bug
|
Hi @camilleislasse, thanks for working on this - is the best next step! 👍 Can you add your findings about when |
|
|
I get the note, but couldn't we make it work to support both? |
I have find a way : By changing the closure signature from: To: Symfony now scans both class-level attributes (invokable pattern) and method-level attributes (method-based pattern). The |
camilleislasse
force-pushed
the
fix/mcp-di-invokable-pattern
branch
4 times, most recently
from
13cab83 to
bd87ca4
Compare
camilleislasse
changed the title
…pport
This commit fixes critical issues preventing MCP tools with constructor
dependencies from working and enables method-based attributes support.
Changes:
- Fix McpPass to use Reference objects for ServiceLocator registration
Previously passed tag arrays directly, causing crashes when tools had
constructor dependencies
- Fix McpBundle::registerMcpAttributes() closure signature
Added missing object $attribute and \Reflector $reflector parameters.
Without these parameters, Symfony's AttributeAutoconfigurationPass only
scans class-level attributes and skips method-level ones entirely
- Add LoggerInterface to CurrentTimeTool to demonstrate DI works
- Update documentation to reflect both patterns are supported
Added examples showing invokable and method-based patterns both work
with full DI support
- Improve McpPassTest to verify ServiceLocator uses References
Previous tests only checked presence, not type of values
camilleislasse
force-pushed
the
fix/mcp-di-invokable-pattern
branch
from
bd87ca4 to
dc04f24
Compare
|
That's great news - thanks @camilleislasse! |
docs/bundles/mcp-bundle.rst
Outdated
| .. warning:: | ||
|
|
||
| The MCP Bundle also ** supports the invokable pattern** where attributes are placed on classes | ||
| with an ``__invoke()`` method. | ||
|
|
||
| ** Invokable ** : | ||
|
|
||
| #[McpTool(name: 'my-tool')] | ||
| class MyTool | ||
| { | ||
| public function __invoke(string $param): string | ||
| { | ||
| // Implementation | ||
| } | ||
| } | ||
|
|
||
| ** Method Based ** : | ||
|
|
||
| class MyTools | ||
| { | ||
| #[McpTool(name: 'tool-one')] | ||
| public function toolOne(): string { } | ||
|
|
||
| #[McpTool(name: 'tool-two')] | ||
| public function toolTwo(): string { } | ||
| } | ||
|
|
There was a problem hiding this comment.
This should be a regular section like "Defining Capabilities" or "Attribute Usage" or other, instead of a warning
|
does it work for you though? |
Yes working, I look 
|
4 participants
** [MCP Bundle] Fix dependency injection and method-based / invokable support**
This commit fixes critical issues preventing MCP tools with constructor
dependencies from working and enables method-based attributes support.
Changes:
Fix McpPass to use Reference objects for ServiceLocator registration
Previously passed tag arrays directly, causing crashes when tools had
constructor dependencies
Fix McpBundle::registerMcpAttributes() closure signature
Added missing object $attribute and \Reflector $reflector parameters.
Without these parameters, Symfony's AttributeAutoconfigurationPass only
scans class-level attributes and skips method-level ones entirely
Add LoggerInterface to CurrentTimeTool to demonstrate DI works
Update documentation to reflect both patterns are supported
Added examples showing invokable and method-based patterns both work
with full DI support
Improve McpPassTest to verify ServiceLocator uses References
Previous tests only checked presence, not type of values